Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Desktop deeplink #13606

Merged
merged 19 commits into from
Jan 9, 2023
Merged

Conversation

cowboycito
Copy link
Contributor

@cowboycito cowboycito commented Dec 15, 2022

Details

Implemented deeplink on the web app that will show a popup to the user asking if they want to open the desktop app (if the desktop app is installed).
The deeplink code will trigger the popup if the user opens up the web app in any of the following routes:

/concierge/*
/statements/*
/partners/plaid/oauth_ios/
/enable-payments/*
/iou/*
/bank-account/*
/v/*
/details/*
/setpassword/*
/settings/*
/r/*

Fixed Issues

$ #10893
PROPOSAL: #10893 (comment)

Tests

  1. Navigate to Expensify's home page on MacOS
  2. Make sure the deeplink popup does NOT show
  3. Navigate to any of the screens listed above, like the settings page
  4. Now you should see a screen saying Launching Expensify and the deeplink popup above it.
  5. If you click to open Expensify it should open the desktop app and take you straight to the given page you are (like the settings page we used in the example above).
  6. If you close the popup and press to "open this link in your browser" if should take you straight to that same page and the "Launching Expensify" screen should be gone.
  7. Please notice that if you're testing in Safari there's a small bug that was already discussed here. Safari has a known bug where the deeplink "check" (the "Launching Expenfify" screen) will only work/show if you move your mouse while the website is loading). That's a minor bug that was previously discussed and that does not affect the deeplink feature, as it will launch the app if you have it installed.
  • Verify that no errors appear in the JS console

Offline tests

  1. Navigate to Expensify's home page on MacOS
  2. Make sure the deeplink popup does NOT show
  3. Navigate to any of the screens listed above, like the settings page
  4. Now you should see a screen saying Launching Expensify and the deeplink popup above it.
  5. If you click to open Expensify it should open the desktop app and take you straight to the given page you are (like the settings page we used in the example above).
  6. If you close the popup and press to "open this link in your browser" if should take you straight to that same page and the "Launching Expensify" screen should be gone.
  7. Please notice that if you're testing in Safari there's a small bug that was already discussed here. Safari has a known bug where the deeplink "check" (the "Launching Expenfify" screen) will only work/show if you move your mouse while the website is loading). That's a minor bug that was previously discussed and that does not affect the deeplink feature, as it will launch the app if you have it installed.

QA Steps

  1. Navigate to Expensify's home page on MacOS
  2. Make sure the deeplink popup does NOT show
  3. Navigate to any of the screens listed above, like the settings page
  4. Now you should see a screen saying Launching Expensify and the deeplink popup above it.
  5. If you click to open Expensify it should open the desktop app and take you straight to the given page you are (like the settings page we used in the example above).
  6. If you close the popup and press to "open this link in your browser" if should take you straight to that same page and the "Launching Expensify" screen should be gone.
  7. Please notice that if you're testing in Safari there's a small bug that was already discussed here. Safari has a known bug where the deeplink "check" (the "Launching Expenfify" screen) will only work/show if you move your mouse while the website is loading). That's a minor bug that was previously discussed and that does not affect the deeplink feature, as it will launch the app if you have it installed.
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Chrome chrome
Safari safari
Microsoft Edge edge
Firefox firefox
Opera opera
iOS
ios.mov
iOS web
ios_mweb.mp4
Android web
android.mweb.mp4
Desktop
desktop.mp4

@cowboycito cowboycito requested a review from a team as a code owner December 15, 2022 00:44
@melvin-bot melvin-bot bot requested review from NikkiWines and Santhosh-Sellavel and removed request for a team December 15, 2022 00:44
@melvin-bot
Copy link

melvin-bot bot commented Dec 15, 2022

@NikkiWines @Santhosh-Sellavel One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@github-actions
Copy link
Contributor

github-actions bot commented Dec 15, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Dec 15, 2022

@cowboycito commits are not signed

So you need to retroactively sign commits on this branch. We are unable to merge pull requests which contain any unsigned commits.

I think that the easiest way to do that would be to follow the instructions in this article.

@NikkiWines NikkiWines added the Waiting for copy User facing verbiage needs polishing label Dec 15, 2022
Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some Spanish translations, but would be good if @Gonals or @iwiznia can double check :)

src/languages/es.js Outdated Show resolved Hide resolved
src/languages/es.js Outdated Show resolved Hide resolved
src/languages/es.js Outdated Show resolved Hide resolved
src/languages/es.js Outdated Show resolved Hide resolved
src/languages/es.js Outdated Show resolved Hide resolved
Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a HL once-over, will give this a proper review next week

src/styles/fontFamily/index.js Outdated Show resolved Hide resolved
desktop/main.js Outdated Show resolved Hide resolved
src/components/DeeplinkWrapper/DeeplinkWrapper.website.js Outdated Show resolved Hide resolved
src/styles/styles.js Outdated Show resolved Hide resolved
src/pages/signin/TermsAndLicenses.js Outdated Show resolved Hide resolved
src/components/DeeplinkWrapper/deeplinkRoutes.js Outdated Show resolved Hide resolved
@cowboycito cowboycito force-pushed the iss-10893/desktop-deeplink branch 2 times, most recently from 26a5b14 to 1425f23 Compare December 17, 2022 01:09
@cowboycito
Copy link
Contributor Author

Fixed all the issues reported by @NikkiWines, signed all the commits (cc @Santhosh-Sellavel) and applied the Spanish translations provided by @aldo-expensify.

@shawnborton
Copy link
Contributor

For this one, I would just use our current ExpensifyNeue font for the headline. We will follow up separately with a PR to add in the correct headline font, but no need to worry about it here.

@cowboycito
Copy link
Contributor Author

For this one, I would just use our current ExpensifyNeue font for the headline. We will follow up separately with a PR to add in the correct headline font, but no need to worry about it here.

Done!

@shawnborton
Copy link
Contributor

Great! Mind updating the screenshots when you get a moment? Thanks!

@Santhosh-Sellavel
Copy link
Collaborator

Thanks @cowboycito

Can you update screen recording for other platforms too, Let's ensure that it's working as expected and no weird behaviors.

@cowboycito
Copy link
Contributor Author

cowboycito commented Dec 21, 2022

@Santhosh-Sellavel sorry for the delay.

Here are the recordings with the style adjustments.

Chrome:

chrome.mov

Safari (showing when the deeplink check works and when it doesn't):

safari.mov

Is there anything else you want me to do so we can start reviewing the code? Also, what is the CLA check that is failing in the PR? How do I fix that?

@shawnborton
Copy link
Contributor

Looking good!

cc @luacmartins as we'll want to update the headline font for this screen too. So depending on whether or not this gets merged before or after your fonts PR, we can adjust accordingly.

@luacmartins luacmartins mentioned this pull request Dec 21, 2022
53 tasks
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks for the changes!

Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great :shipit:!! Thanks for your perseverance here @cowboycito & @Santhosh-Sellavel 🙌

One minor note for the testing steps @cowboycito :

Please notice that if you're testing in Safari there's a small bug that was already discussed #10893 (comment).

Might be good to add a bit more context here as to what the small bug is, so you don't have to go to a separate page to know if you've encountered the bug while testing.

@cowboycito
Copy link
Contributor Author

Looks great :shipit:!! Thanks for your perseverance here @cowboycito & @Santhosh-Sellavel 🙌

One minor note for the testing steps @cowboycito :

Please notice that if you're testing in Safari there's a small bug that was already discussed #10893 (comment).

Might be good to add a bit more context here as to what the small bug is, so you don't have to go to a separate page to know if you've encountered the bug while testing.

@NikkiWines done

@luacmartins
Copy link
Contributor

@cowboycito seems like the author checklist is outdated. Could you please update it with the most recent version?

@cowboycito
Copy link
Contributor Author

@cowboycito seems like the author checklist is outdated. Could you please update it with the most recent version?

@luacmartins done!

Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working fine for me!

@NikkiWines NikkiWines merged commit eb6aa08 into Expensify:main Jan 9, 2023
@OSBotify
Copy link
Contributor

OSBotify commented Jan 9, 2023

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start nativeLaunch 10.419 ms → 19.375 ms (+8.956 ms, +86.0%) 🔴
App start regularAppStart 0.014 ms → 0.015 ms (+0.000 ms, +3.2%)
App start runJsBundle 195.563 ms → 195.188 ms (-0.375 ms, ±0.0%)
App start TTI 689.824 ms → 683.859 ms (-5.964 ms, -0.9%)
Open Search Page TTI 605.695 ms → 599.020 ms (-6.675 ms, -1.1%)
Show details
Name Duration
App start nativeLaunch Baseline
Mean: 10.419 ms
Stdev: 2.137 ms (20.5%)
Runs: 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 10 10 10 11 11 11 11 11 12 12 12 13 14 15 15 15

Current
Mean: 19.375 ms
Stdev: 1.452 ms (7.5%)
Runs: 17 18 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 21 22 22 22 23
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (9.6%)
Runs: 0.012165999971330166 0.012533000204712152 0.012654999736696482 0.012817000038921833 0.012979999650269747 0.012980000115931034 0.013061000034213066 0.01310200011357665 0.013141999952495098 0.013264999724924564 0.013264999724924564 0.013387000188231468 0.013590000104159117 0.013591000344604254 0.013631999958306551 0.013712000101804733 0.013712999876588583 0.013875999953597784 0.014322999864816666 0.014404000248759985 0.014607999939471483 0.014688999857753515 0.0148930000141263 0.015015000011771917 0.015462000388652086 0.015503000002354383 0.0157880000770092 0.0166830001398921 0.01696800021454692 0.0176189998164773

Current
Mean: 0.015 ms
Stdev: 0.001 ms (6.7%)
Runs: 0.013061000034213066 0.01310200011357665 0.013143000192940235 0.013427999801933765 0.013549999799579382 0.013590000104159117 0.013631000183522701 0.013753000181168318 0.013793999794870615 0.0138349998742342 0.0138349998742342 0.014159999787807465 0.014322999864816666 0.014403999783098698 0.01448499970138073 0.014485999941825867 0.014607000164687634 0.014730000402778387 0.014932999853044748 0.014973999932408333 0.014973999932408333 0.015015000011771917 0.015176999848335981 0.015217999927699566 0.015217999927699566 0.01537999976426363 0.015462000388652086 0.01582799991592765 0.016398000065237284 0.016398000065237284 0.016681999899446964
App start runJsBundle Baseline
Mean: 195.563 ms
Stdev: 29.261 ms (15.0%)
Runs: 153 157 157 158 160 160 169 171 172 176 177 178 180 188 188 192 198 198 198 201 202 207 209 213 226 228 229 232 235 236 252 258

Current
Mean: 195.188 ms
Stdev: 27.236 ms (14.0%)
Runs: 161 162 165 167 168 168 169 170 172 173 176 181 182 183 183 189 191 196 196 199 207 207 208 208 209 217 218 221 235 250 256 259
App start TTI Baseline
Mean: 689.824 ms
Stdev: 35.442 ms (5.1%)
Runs: 621.8982939999551 636.1714320001192 636.8211759999394 641.930933999829 646.2069580000825 648.0137370000593 661.7014139997773 663.8602849999443 665.5828860001639 666.6015329998918 672.803501999937 673.5715230000205 675.385766999796 676.1013930002227 676.6811819998547 680.0793579998426 693.1834470001049 696.8697250001132 700.9469010001048 702.8283350002021 711.8136200001463 712.8503220002167 713.565950000193 715.8364479998127 720.0107049997896 724.8877639998682 726.7857630001381 730.4356459998526 737.2338640000671 746.1708809998818 747.1621659998782 750.3609409998171

Current
Mean: 683.859 ms
Stdev: 29.709 ms (4.3%)
Runs: 640.25014499994 641.1255899998359 649.9429709999822 651.056596999988 653.2129600001499 653.6327479998581 655.718499999959 659.5019620000385 662.0689309998415 662.4442440001294 672.0789049998857 673.3177820001729 673.7223410001025 674.3884069998749 674.4115109997801 674.8843840002082 677.5897450000048 681.3742669997737 682.9315740000457 695.9422470000573 699.6625879998319 700.1576140001416 701.5209690001793 703.0622419998981 704.0587599999271 711.4851890001446 713.1089460002258 724.8497939999215 730.3920550001785 737.0656989999115 764.6819620002061
Open Search Page TTI Baseline
Mean: 605.695 ms
Stdev: 25.671 ms (4.2%)
Runs: 548.7828780002892 568.8864750000648 570.6480310000479 577.5766610000283 578.3484300002456 578.5702720000409 579.13830600027 583.9148359997198 590.2533780001104 590.3605960002169 591.943847999908 592.5712490002625 592.7895510001108 597.4974360000342 598.2146399999037 599.7218019999564 600.3895270000212 600.8952640001662 608.1415200000629 613.5559489997104 613.6378179998137 615.4777839998715 619.7522789998911 626.6169429998845 629.0522060003132 632.9851080002263 634.7223720001057 634.9782309997827 634.9927989998832 638.6016039997339 646.4793700003065 646.643962000031 651.7929689995944

Current
Mean: 599.020 ms
Stdev: 21.965 ms (3.7%)
Runs: 567.3149419999681 567.3254399998114 568.2692879997194 568.6654059998691 569.6825759997591 571.0745040001348 577.3090409999713 579.6863199998625 583.3933520000428 583.7967530000024 588.6802159999497 588.7269290001132 588.9259850000963 591.9107259996235 595.9408780001104 598.0925300000235 598.7986659999005 599.2847090000287 599.4692799998447 600.2740890001878 604.2049970002845 604.2885739998892 606.5966799999587 610.0465499996208 614.2207030002028 615.5767830000259 622.8831380000338 627.7417399999686 630.206298999954 632.269288000185 634.7617190000601 639.0941159999929 639.1329749999568

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Jan 9, 2023
@luacmartins
Copy link
Contributor

Started a discussion about the performance regression here

@cowboycito
Copy link
Contributor Author

Started a discussion about the performance regression here

@luacmartins let me know it I can/need to do anything as I don't have access to that Slack link

@luacmartins
Copy link
Contributor

Seems like we don't care about the meaningless changes to duration, so I'll remove the label and work on a fix to prevent adding the label in such cases.

@luacmartins luacmartins removed the DeployBlockerCash This issue or pull request should block deployment label Jan 9, 2023
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @NikkiWines in version: 1.2.52-0 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.2.52-4 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Jan 15, 2023

Sorry everyone,
Only today I was able to test this on Production,

I believe this is not working as expected, @cowboycito

Screenshot 2023-01-15 at 9 15 54 PM

cc: @NikkiWines

@cowboycito
Copy link
Contributor Author

@Santhosh-Sellavel check out this thread

@Santhosh-Sellavel
Copy link
Collaborator

I'll dm you.

@cowboycito
Copy link
Contributor Author

@Santhosh-Sellavel screenshots of both staging and production after removing all references to Electron in any node_modules I had on my computer:

Staging:
Screen Shot 2023-01-15 at 12 58 14

Production:
Screen Shot 2023-01-15 at 12 59 00

@Santhosh-Sellavel
Copy link
Collaborator

Ya, that helps, thanks @cowboycito. I still feel it's quite annoying for developers here.


openRouteInDesktopApp() {
const expensifyUrl = new URL(CONFIG.EXPENSIFY.NEW_EXPENSIFY_URL);
const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}${window.location.pathname}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We forgot to pass the query params here which caused a few URLs to break in our App.

Here is the regression issue #15059

cc: @Santhosh-Sellavel

@rushatgabhane
Copy link
Member

rushatgabhane commented May 5, 2023

we'd need a solution that works for any link that a user would open in a browser (source: #10893 (comment))

Hi all, from the issue the plan was that clicking any link would deeplink with desktop.
But somehow we ended up enabling deeplink only for a small set of links, causing #16083

Cheers!

}
}

isMacOSWeb() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we put this in the component instead of the Browser lib?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No specific reasons we just missed out on that Review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants